Avoid rooting cryptography through ZipArchive on browser - #130688
Avoid rooting cryptography through ZipArchive on browser#130688alinpahontu2912 with Copilot wants to merge 12 commits into
Conversation
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara |
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAes.PlatformNotSupported.cs:9
WinZipAesKeyMaterialis declared with a trailing;(a forward declaration), which is not valid C# syntax and will fail compilation for browser TFMs. Define an empty struct body instead so the browser stub can satisfy references from the rest of the ZIP code.
internal readonly struct WinZipAesKeyMaterial;
| } | ||
| } | ||
| finally | ||
| { | ||
| CryptographicOperations.ZeroMemory(passwordBytes); | ||
| Array.Clear(passwordBytes); | ||
| ArrayPool<byte>.Shared.Return(passwordBytes); | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.IO.Compression/src/System/IO/Compression/WinZipAes.PlatformNotSupported.cs:9
internal readonly struct WinZipAesKeyMaterial;is not valid C# syntax (C# doesn't support forward declarations). This will fail to compile the browser target. Define an (empty) struct type instead so the rest of the code can reference it.
internal readonly struct WinZipAesKeyMaterial;
src/libraries/System.IO.Compression/src/System/IO/Compression/ZipCryptoStream.Random.BrowserOrWasi.cs:18
- PR description says ZipCrypto header randomness is generated with
Guid.NewGuid(), but the implementation usesInterop.Sys.GetCryptographicallySecureRandomByteson browser/wasi (andRandomNumberGenerator.Fillelsewhere). Please update the PR description (or code) so the documented approach matches what actually shipped.
// System.Security.Cryptography's RandomNumberGenerator is not referenced on browser and is
// not supported on wasi, so call the shared native CSPRNG directly. It is backed by
// crypto.getRandomValues() on browser and by wasi:random (getentropy/__wasi_random_get) on wasi.
Span<byte> randomBytes = header.Slice(0, 10);
fixed (byte* pRandomBytes = randomBytes)
{
if (Interop.Sys.GetCryptographicallySecureRandomBytes(pRandomBytes, randomBytes.Length) != 0)
{
throw new IOException(SR.UnableToGenerateRandomBytes);
…atforms Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
| @@ -0,0 +1,13 @@ | |||
| // Licensed to the .NET Foundation under one or more agreements. | |||
There was a problem hiding this comment.
instead reuse it
<Compile Include="$(CommonPath)Interop\Unix\System.Native\Interop.GetRandomBytes.cs"
Link="Common\Interop\Unix\System.Native\Interop.GetRandomBytes.cs" />
There was a problem hiding this comment.
I think with Interop.GetCryptographicallySecureRandomBytes you can drop crypto assembly dependency on all platforms, not just browser and have unified code.
There was a problem hiding this comment.
I think the cryptography dependency is still preferred. It provides a clearer abstraction, avoids exposing interop details, and keeps the implementation consistent with the rest of .NET. I'd rather find an acceptable browser-specific alternative for ZipCrypto to address the size regession than switch everything over to Interop.GetCryptographicallySecureRandomBytes.
Co-authored-by: alinpahontu2912 <56953855+alinpahontu2912@users.noreply.github.com>
Fixes #130650
which is regression from #122093
Using
ZipArchiveon browser-wasm unnecessarily retainedSystem.Security.Cryptography, increasing trimmed application size even though WinZip AES is unsupported there.Changes
PlatformNotSupportedException.Guid.NewGuid().Array.Clear.System.Security.Cryptography.